-
Notifications
You must be signed in to change notification settings - Fork 2
p_sync Milestone 1: Create Sync Client, implement pull operation, CLI, and overloads for log + call #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Sync client requires dotenv at runtime
| line-length = 120 | ||
|
|
||
| [tool.poetry.scripts] | ||
| humanloop = "humanloop.cli.__main__:cli" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
integration test: use subprocess to check if cli works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if exit code 0 when calling cli command; maybe check for exit code 1 when api key not provided
src/humanloop/cli/__main__.py
Outdated
| \b | ||
| This command will: | ||
| 1. Fetch prompt and agent files from your Humanloop workspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 1. Fetch prompt and agent files from your Humanloop workspace | |
| 1. Fetch serializable files from your Humanloop workspace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided against this as I don't think users will understand what 'serializable' means.
src/humanloop/cli/__main__.py
Outdated
| successful_files = sync_client.pull(path, environment) | ||
|
|
||
| # Get metadata about the operation | ||
| metadata = sync_client.metadata.get_last_operation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow-up with a loom
src/humanloop/cli/__main__.py
Outdated
| for file in metadata['successful_files']: | ||
| click.echo(click.style(f" ✓ {file}", fg=SUCCESS_COLOR)) | ||
| else: | ||
| files_to_display = metadata['successful_files'][:MAX_FILES_TO_DISPLAY] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if len(failed_failes) > 0, cancel the operation, inform user via message in terminal, exit 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I'm seeing with this is it would make things quite complex to support transactions, i.e. allow rollbacks if one file fails.
I think there is an error that we don't handle properly. Would expect to see "humanloop.core.api_error.ApiError: status_code: 401, body: {'detail': 'Could not validate credentials.'}" as first line, not |
| version = "0.8.36" | ||
| description = "" | ||
| version = "0.8.39" | ||
| description = "Humanloop Python SDK" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you changes this? this is auto-generated by fen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file's autogenerated by fern? Oops, thought we'd be able to modify project metadata.
| @@ -0,0 +1,2 @@ | |||
| [pytest] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: i have updated humanloop-docs with the pytest-xdist dependency; we'll regenerate the sdk and rebase this PR on top of it before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🙏
4b70868 to
76e8ac9
Compare

Superseded by PR 69
p_sync/base is the auto-generated SDK with my latest backend changes from here. Did this to make the PR focused on the custom code changes.
pullcommand. Uses the click library.call()and.log()functions to use serialized contents of.prompt/.agentlocal files whenuse_local_filesis set totruewhen initializing the humanloop client.use_local_files=Trueand tries to reference path + remote-specific information (version_id/environment).How to Use Locally
Prerequisites
Run this backend branch locally
Set up the SDK
Clone this branch
git clone --branch p_sync/pull git@github.com:humanloop/humanloop-python.gitInstall dependencies
poetry installActivate the venv
poetry shellTest the CLI
In the same directory as this cloned SDK
humanloop pull --base-url http://localhost:80/v5In another project:
poetry initUse default configuration>=3.9,<43a. Get the absolute path to the local SDK using
pwd3b. In your playground project run
poetry add --editable <path to SDK>poetry installpoetry shell) you can test the CLIhumanloop pull --base-url http://localhost:80/v5Note that you can remove
--base-urlentirely as the relevant backend changes are live in prod.